feat(llm): better thinking mode control#608
Conversation
Signed-off-by: Richard Chien <stdrc@outlook.com>
stdrc
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors the thinking mode architecture by moving thinking state management from the KimiSoul class to the LLM/ChatProvider level, providing more robust and flexible control over thinking mode across the application.
Changes:
- Adds
thinking_effortproperty toChatProviderprotocol and implements it across all chat providers - Adds
thinkingparameter tocreate_llm()for configuration at LLM creation time - Migrates thinking state from
metadata.jsontoconfig.tomlwithdefault_thinkingfield - Replaces Tab key toggle with
/modelcommand for switching thinking mode - Adds
ModelInfoPydantic model for parsing API responses withsupports_reasoning - Adds
always_thinkingcapability for models that always use thinking mode
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kimi_cli/soul/init.py | Added thinking property to Soul protocol |
| src/kimi_cli/soul/kimisoul.py | Replaced set_thinking() method with thinking property that reads from chat provider |
| src/kimi_cli/llm.py | Added thinking parameter to create_llm() and always_thinking capability |
| src/kimi_cli/config.py | Added default_thinking field and migration logic from metadata |
| src/kimi_cli/metadata.py | Configured to ignore extra fields (allows old thinking field) |
| src/kimi_cli/platforms.py | Added ModelInfo Pydantic model for parsing API responses |
| src/kimi_cli/ui/shell/slash.py | Rewrote /model command to support thinking mode selection |
| src/kimi_cli/ui/shell/setup.py | Added thinking mode prompt in setup flow |
| src/kimi_cli/ui/shell/prompt.py | Removed Tab key binding and _toast_thinking() function |
| src/kimi_cli/ui/shell/init.py | Updated to use soul.thinking property |
| src/kimi_cli/cli/init.py | Moved --thinking option and removed metadata-based thinking state |
| src/kimi_cli/app.py | Updated to use config-based thinking with CLI override |
| src/kimi_cli/acp/server.py | Updated to use config.default_thinking instead of metadata |
| packages/kosong/src/kosong/chat_provider/*.py | Implemented thinking_effort property in all providers |
| docs/**/*.md | Updated documentation for new thinking mode workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| curr_thinking = soul.thinking | ||
|
|
||
| # Step 1: Select model | ||
| model_choices: list[tuple[str, str]] = [] | ||
| for name in sorted(config.models): | ||
| model_cfg = config.models[name] | ||
| provider_label = get_platform_name_for_provider(model_cfg.provider) or model_cfg.provider | ||
| marker = " (current)" if name == curr_model_name else "" | ||
| label = f"{model_cfg.model} ({provider_label}){marker}" | ||
| model_choices.append((name, label)) | ||
|
|
||
| try: | ||
| selected_model_name = await ChoiceInput( | ||
| message="Select a model (↑↓ navigate, Enter select, Ctrl+C cancel):", | ||
| options=model_choices, | ||
| default=curr_model_name or model_choices[0][0], | ||
| ).prompt_async() | ||
| except (EOFError, KeyboardInterrupt): | ||
| return | ||
|
|
||
| if not selected_model_name: | ||
| return | ||
|
|
||
| selected_model_cfg = config.models[selected_model_name] | ||
| selected_provider = config.providers.get(selected_model_cfg.provider) | ||
| if selected_provider is None: | ||
| console.print(f"[red]Provider not found: {selected_model_cfg.provider}[/red]") | ||
| return | ||
|
|
||
| # Step 2: Determine thinking mode | ||
| capabilities = derive_model_capabilities(selected_model_cfg) | ||
| new_thinking: bool | ||
|
|
||
| if "always_thinking" in capabilities: | ||
| new_thinking = True | ||
| elif "thinking" in capabilities: | ||
| thinking_choices: list[tuple[str, str]] = [ | ||
| ("off", "off" + (" (current)" if not curr_thinking else "")), | ||
| ("on", "on" + (" (current)" if curr_thinking else "")), | ||
| ] | ||
| try: | ||
| selection = await ChoiceInput( | ||
| message=("Select a model to switch to (↑↓ navigate, Enter select, Ctrl+C cancel):"), | ||
| options=choices, | ||
| default=current_model_name or choices[0][0], | ||
| thinking_selection = await ChoiceInput( | ||
| message="Enable thinking mode? (↑↓ navigate, Enter select, Ctrl+C cancel):", | ||
| options=thinking_choices, | ||
| default="on" if curr_thinking else "off", | ||
| ).prompt_async() | ||
| except (EOFError, KeyboardInterrupt): | ||
| return | ||
|
|
||
| if not selection: | ||
| if not thinking_selection: | ||
| return | ||
|
|
||
| model_name = selection | ||
| new_thinking = thinking_selection == "on" | ||
| else: | ||
| try: | ||
| parsed_args = shlex.split(raw_args) | ||
| except ValueError: | ||
| console.print("[red]Usage: /model <name>[/red]") | ||
| return | ||
| if len(parsed_args) != 1: | ||
| console.print("[red]Usage: /model <name>[/red]") | ||
| return | ||
| model_name = parsed_args[0] | ||
| if model_name not in config.models: | ||
| console.print(f"[red]Unknown model: {model_name}[/red]") | ||
| return | ||
| new_thinking = False | ||
|
|
||
| if current_model_name == model_name: | ||
| console.print(f"[yellow]Already using model {model_name}.[/yellow]") | ||
| return | ||
| # Check if anything changed | ||
| model_changed = curr_model_name != selected_model_name | ||
| thinking_changed = curr_thinking != new_thinking |
There was a problem hiding this comment.
When soul.thinking returns None (if LLM is not set or thinking mode is not explicitly set), the comparison curr_thinking != new_thinking at line 232 will compare None with a boolean value. This could lead to incorrect detection of whether thinking has changed. For example, if curr_thinking is None and new_thinking is False, they will be considered different even though both represent "thinking is off". The code should handle the None case explicitly, treating it as False for comparison purposes.
There was a problem hiding this comment.
Not applicable - soul.thinking always returns bool, never None. See KimiSoul.thinking property implementation.
| thinking_choices: list[tuple[str, str]] = [ | ||
| ("off", "off" + (" (current)" if not curr_thinking else "")), | ||
| ("on", "on" + (" (current)" if curr_thinking else "")), | ||
| ] | ||
| try: | ||
| selection = await ChoiceInput( | ||
| message=("Select a model to switch to (↑↓ navigate, Enter select, Ctrl+C cancel):"), | ||
| options=choices, | ||
| default=current_model_name or choices[0][0], | ||
| thinking_selection = await ChoiceInput( | ||
| message="Enable thinking mode? (↑↓ navigate, Enter select, Ctrl+C cancel):", | ||
| options=thinking_choices, | ||
| default="on" if curr_thinking else "off", |
There was a problem hiding this comment.
When displaying the current thinking mode marker in the choices, the code uses not curr_thinking and curr_thinking directly. However, curr_thinking can be None, which will be treated as falsy. This means when curr_thinking is None, the "(current)" marker will appear on "off", but conceptually None means "not explicitly set" rather than "explicitly off". This could be misleading to users when switching between models where thinking mode wasn't previously set.
There was a problem hiding this comment.
Not applicable - same as above, curr_thinking = soul.thinking is always bool.
| if thinking_config["type"] == "disabled": | ||
| return "off" | ||
| budget = thinking_config["budget_tokens"] | ||
| if budget <= 1024: | ||
| return "low" | ||
| if budget <= 4096: | ||
| return "medium" | ||
| return "high" |
There was a problem hiding this comment.
The thinking_effort property assumes that when thinking_config["type"] is not "disabled", it will have a "budget_tokens" key. However, if the type is something else (e.g., "enabled" without budget_tokens being set), accessing thinking_config["budget_tokens"] will raise a KeyError. The code should use .get("budget_tokens") with a default value or check for the key's existence before accessing it.
There was a problem hiding this comment.
Not applicable - the code already uses .get("budget_tokens", 0) with a default value.
| def derive_model_capabilities(model: LLMModel) -> set[ModelCapability]: | ||
| capabilities = model.capabilities or set() | ||
| if model.model in {"kimi-for-coding", "kimi-code"} or "thinking" in model.model: | ||
| # Models with "thinking" in their name are always-thinking models | ||
| if "thinking" in model.model.lower() or "reason" in model.model.lower(): | ||
| capabilities.update(("thinking", "always_thinking")) | ||
| # These models support thinking but can be toggled on/off | ||
| elif model.model in {"kimi-for-coding", "kimi-code"}: | ||
| capabilities.add("thinking") | ||
| return capabilities |
There was a problem hiding this comment.
The function derive_model_capabilities mutates the capabilities set obtained from model.capabilities. If model.capabilities is not None, line 207 gets a reference to it, and lines 210 and 213 directly mutate this set. This could lead to unintended side effects where the model's capabilities are modified. The function should create a copy of the capabilities set before modifying it, e.g., capabilities = set(model.capabilities or []).
Summary
Refactors thinking mode architecture by moving thinking state from KimiSoul to LLM/ChatProvider level.
Changes
ChatProvider Protocol
thinking_effortproperty returningThinkingEffort | NoneNone= not explicitly set (default behavior)"off"= explicitly disabled"low"/"medium"/"high"= explicitly enabledLLM Creation
create_llm()now acceptsthinking: bool | NoneparameterSoul Layer
thinking: boolproperty to Soul protocolKimiSoul.thinkingnow reads fromchat_provider.thinking_effortset_thinking()method from KimiSoul/model Command
curr_model_cfg,curr_model_name,selected_model_name)/setup Command
ModelInfo.capabilitiesfrom API responseConfig
default_thinking: boolto Configmetadata.thinkingtoconfig.default_thinkingMetadatanow allows extra fields (ignores oldthinkingfield)ModelInfo
platforms.pysupports_reasoningfrom API responsecapabilitiesproperty derives thinking capabilitiesCLI
--thinkingoption to be right after--model--modelstyleRemoved
_toast_thinking()function